Merge common code bases for TdsParserStateObject.cs (2) #1844

panoskj · 2022-11-22T01:06:38Z

Second part of #1520. As I explained in this comment, #1843 does more or less the same, but I have already written the code for this PR. Perhaps you will find some use for it.

As for the code itself, you will notice I have left lots of #if directives, intentionally, to highlight the differences between the two versions, before deciding how to resolve them.

PS: either way, please try to merge one of these PRs as soon as possible. They will allow us to partially fix the async bug. A full fix would require ~~SqlDataReader~~ TdsParser too, if I remember correctly.

… parts of dotnet#667).

…arts of dotnet#285 to netfx).

…erStateObject.

…ged behavior of CheckConnection for netfx, to make it same as netcore, affecting IsConnectionAlive).

dotnet#378 and dotnet#528 to netfx).

…erStateObject.

…ds of TdsParserStateObject.

bhugot · 2022-11-22T01:43:05Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

 internal partial class TdsParserStateObject
 {
+ private static bool UseManagedSNI => false;
+
 private SNIHandle _sessionHandle = null; // the SNI handle we're to work on


Should'nt this be removed to be replase by SessionHandle struct

Do you mean to replace SNIHandle with SessionHandle struct at line 32?

If yes, you can't do this because I have defined it as readonly ref. I needed to "add" the IsNull property to SNIHandle, so I encapsulated it in a struct. Then I added readonly ref so that this struct never allocates memory and is copied as if it was a plain reference. This is an optimization which is probably not needed, but I tried to make as little behavioral/performance changes as possible while refactoring.

Edit: to clarify, SessionHandle struct is only created and used in certain methods. There is a readonly ref SessionHandle in netcore version used in a similar way.

Ok I see. I was asking myself about encapsulating like in netcore version that why I have read it too fast. Thanks for clarification

codecov · 2022-11-22T01:55:26Z

Codecov Report

Patch coverage: 96.12% and project coverage change: +2.57 🎉

Comparison is base (a4f18ca) 70.82% compared to head (2e2dab5) 73.40%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
+ Coverage   70.82%   73.40%   +2.57%     
==========================================
  Files         292      224      -68     
  Lines       61777    36213   -25564     
==========================================
- Hits        43752    26581   -17171     
+ Misses      18025     9632    -8393

Flag	Coverage Δ
addons	`92.88% <ø> (+0.49%)`	⬆️
netcore	`73.23% <96.12%> (-1.21%)`	⬇️
netfx	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/Microsoft/Data/SqlClient/TdsParserSessionPool.cs	`91.78% <ø> (+5.53%)`	⬆️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	`88.85% <95.55%> (+1.39%)`	⬆️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	`87.26% <96.66%> (+42.97%)`	⬆️
...SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs	`69.35% <100.00%> (+1.10%)`	⬆️

... and 201 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lcheunglci · 2022-11-22T15:14:51Z

Thanks @panoskj for making each method merge as a separate commit so it's easier to review especially when working with a fairly large file :)

panoskj · 2022-11-23T07:44:03Z

@lcheunglci I'm glad you find smaller commits useful.

It should be mentioned that I wrote this code 7-8 months ago and now rebased it on the latest version. But meanwhile, I added some style changes in #1520. As a result, some of these style changes are missing in the merged file of this PR. It may look like a regression, but I intend to add another commit that brings them back again.

…Removed unnecessary casts. Removed unnecessary assignment. Added readonly specifier where possible.

bhugot

some quick change without changing behavior

bhugot · 2022-11-23T21:04:04Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

+ private SessionHandle SessionHandle => new SessionHandle(_sessionHandle);
+
+ private bool TransparentNetworkIPResolution => _parser.Connection.ConnectionOptions.TransparentNetworkIPResolution;
+
 internal bool _pendingData = false;


Could you make private this field and use HasPendingData property where it's not used

These fields are used across many files, that's why I didn't touch them. But I now see they will help with merging TdsParser, so I will add them.

Yes that was my point to change them and it's a quick harmless refactoring. Thanks

Just added a commit implementing the requested change.

bhugot · 2022-11-23T21:04:37Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

+ private SessionHandle SessionHandle => new SessionHandle(_sessionHandle);
+
+ private bool TransparentNetworkIPResolution => _parser.Connection.ConnectionOptions.TransparentNetworkIPResolution;
+
 internal bool _pendingData = false;
 internal bool _errorTokenReceived = false; // Keep track of whether an error was received for the result.


Could you make private this field and add the same property with get/set on it as NetCore:

internal bool HasReceivedError { get => _errorTokenReceived; set => _errorTokenReceived= value; }

bhugot · 2022-11-23T21:06:07Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

@@ -35,12 +50,6 @@ internal partial class TdsParserStateObject
 // Timeout variables
 internal bool _attentionReceived = false; // NOTE: Received is not volatile as it is only ever accessed\modified by TryRun its callees (i.e. single threaded access)


Could you make private this field and add the same property with get/set on it as NetCore:

internal bool HasReceivedAttention { get => _attentionReceived; set => _attentionReceived= value; }

bhugot · 2022-11-23T21:06:56Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

- // the code but does not remember either. At some point, we need to research killing this
- // logic.
- private volatile int _allowObjectID;
-
 internal bool _hasOpenResult = false;


Could you make private this field and use HasOpenResult property where it's not used

bhugot · 2022-11-23T21:07:27Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

@@ -51,45 +60,6 @@ internal partial class TdsParserStateObject

 internal bool _receivedColMetaData; // Used to keep track of when to fire StatementCompleted event.


Could you make private this field and add the same property with get/set on it as NetCore:

internal bool HasReceivedColumnMetadata { get => _receivedColMetaData; set => _receivedColMetaData= value; }

…expose the same interface as netcore version. This helps merging other files, such as TdsParser.

…ry assignments. Used discard to ignore out parameters, instead of an unused variable.

panoskj · 2022-11-24T08:42:56Z

Please review this commit with special care. It may change the behavior of IsConnectionAlive method in some edge case for netfx (so that netfx behavior matches netcore). To me it looks like either the netcore version is correct, or both versions are correct (and their behavior is actually the same).

PS: we would have to look into the native driver's code to determine whether the behavior changed or not, that's why I am unsure. The method in question is SNINativeMethodWrapper.SNICheckConnection, specifically in the case it receives a null value.

bhugot · 2023-01-18T08:28:53Z

Any news on merging this @lcheunglci

lcheunglci · 2023-01-19T19:07:26Z

Any news on merging this @lcheunglci

Yes, we'll get to reviewing it after the 5.1.0 release.

…rStateObject-2 # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

Wraith2 · 2023-06-20T23:57:03Z

Can we get an update on this please? there's still a lot of work to be done with merges and this has been open a long time now.

JRahnama · 2023-06-21T20:08:58Z

/azurepipelines run

azure-pipelines · 2023-06-21T20:09:08Z

Azure Pipelines successfully started running 1 pipeline(s).

Wraith2 · 2023-06-21T23:44:56Z

If this is too big or too old a changeset to be reviewed and merged it could be closed and individual pieces of the work re-done in smaller PR's.

JRahnama · 2023-06-22T04:00:57Z

If this is too big or too old a changeset to be reviewed and merged it could be closed and individual pieces of the work re-done in smaller PR's.

I totally agree with you. Let's break this down in several PRs and merge them before the next preview release.

On some other notes, I was considering, just me this is not a team decision or official roadmap, converting the netfx to SDK style project and do some testing for code base merging. That would make our lives a lot easier I think. It would probably will give us a bit more complicated csproj, but one csproj which works for all of them.

JRahnama · 2023-06-22T04:02:54Z

@panoskj will you be able to break this PR to couple of smaller changeset or you prefer someone else to do it?

David-Engel · 2023-06-22T22:25:12Z

On the "breaking it up" suggestion. Troubleshooting the pipeline failures might be easier by breaking it up, too. I'd first try to make changes to the separate TdsParserStateObject.cs files to bring them closer to identical. Ideas for steps in individual PRs:

Rename internal variables/properties to all match.
Reorder methods/member variables to align the file layout as much as possible, keeping identical code together as much as possible. I might even add some regions and/or comments to help future diffs keep their alignment when common code is removed from each file. (No other changes so that reviewers don't have to scrutinize any differences between the large code blocks.)
Move common code into a new common file. Avoid any changes within moved blocks of code.

panoskj · 2023-07-08T07:19:23Z

@panoskj will you be able to break this PR to couple of smaller changeset or you prefer someone else to do it?

@JRahnama @Wraith2 I will be able to split the PR within the next 4 weeks - my free time is very limited right now. Luckily, most commits are independent, so it should be easy to split the PR.

The reason I made it so large, is because I thought it would take you even more time, in total, to review 10 PRs, judging from the time it took you to accept the previous, smaller PR. Also keep in mind this file has several more thousands of lines to be merged once this PR is accepted.

panoskj added 18 commits November 22, 2022 01:27

Merging "Buffer read" methods of TdsParserStateObject (note: includes…

840af41

… parts of dotnet#667).

Merging of TdsParserStateObject.TryReadPlpBytes method (note: ports p…

5429584

…arts of dotnet#285 to netfx).

Merging "Value Skip Logic" methods of TdsParserStateObject.

d112701

Merging some "Network/Packet Reading & Processing" methods of TdsPars…

f115d59

…erStateObject.

Merging of TdsParserStateObject.ReadSniSyncOverAsync method.

7c3dfe2

Merging some "Network/Packet Reading & Processing" methods of TdsPars…

6ae1321

…erStateObject.

Merging TdsParserStateObject.ReadSni method.

e5af755

Merging TdsParserStateObject.IsConnectionAlive method.

49f7169

Merging TdsParserStateObject.ValidateSNIConnection method (note: chan…

7073564

…ged behavior of CheckConnection for netfx, to make it same as netcore, affecting IsConnectionAlive).

Merging TdsParserStateObject.ReadSniError method.

1ddaeec

Merging of TdsParserStateObject.ProcessSniPacket method.

32b2b2f

Merging of TdsParserStateObject.ReadAsyncCallback (note: ports parts of

81f549e

dotnet#378 and dotnet#528 to netfx).

Merging TdsParserStateObject.WriteAsyncCallback method.

ff3d22e

Merging some "Network/Packet Writing & Processing" methods of TdsPars…

567d0e6

…erStateObject.

Merging constructor of TdsParserStateObject.

96c2e68

Merging common type NullBitmap of TdsParserStateObject.

32b2699

Merging StartSession, Cancel and ResetCancelAndProcessAttention metho…

c910c06

…ds of TdsParserStateObject.

Merging DecrementPendingCallbacks and incrementPendingCallbacks metho…

a720efa

…ds of TdsParserStateObject.

bhugot mentioned this pull request Nov 22, 2022

Merge more methods from TdsParserStateObject #1843

Closed

bhugot reviewed Nov 22, 2022

View reviewed changes

lcheunglci requested review from JRahnama, DavoudEshtehari and lcheunglci November 22, 2022 14:22

TdsParserStateObject: Removed unnecessary "#pragma warning disable". …

6a89090

…Removed unnecessary casts. Removed unnecessary assignment. Added readonly specifier where possible.

bhugot reviewed Nov 23, 2022

View reviewed changes

panoskj added 2 commits November 23, 2022 23:42

TdsParserStateObject: replaced some netfx fields with properties, to …

aadf8e3

…expose the same interface as netcore version. This helps merging other files, such as TdsParser.

TdsParserStateObject: removed consecutive empty lines.

bc0ceb2

TdsParserStateObject: Removed trailing whitespaces. Removed unnecessa…

98bd515

…ry assignments. Used discard to ignore out parameters, instead of an unused variable.

bhugot approved these changes Nov 24, 2022

View reviewed changes

panoskj mentioned this pull request Feb 1, 2023

Add and use TryReadChars method #1544

Merged

Merge remote-tracking branch 'remotes/origin/main' into MergeTdsParse…

2e2dab5

…rStateObject-2 # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

DavoudEshtehari added this to the 5.2.0-preview3 milestone Jun 21, 2023

DavoudEshtehari added the ➕ Code Health Changes related to source code improvements label Jun 21, 2023

DavoudEshtehari requested review from Kaur-Parminder and removed request for lcheunglci June 21, 2023 16:11

panoskj closed this Jul 8, 2023

DavoudEshtehari removed this from the 5.2.0-preview3 milestone Jul 19, 2023

panoskj mentioned this pull request Sep 24, 2023

Merge common code bases for TdsParserStateObject.cs (3) #2168

Merged

DavoudEshtehari mentioned this pull request Nov 9, 2023

Possible mergeable classes for merging code bases #1261

Open

panoskj mentioned this pull request Dec 11, 2023

Reading large data (binary, text) asynchronously is extremely slow #593

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge common code bases for TdsParserStateObject.cs (2) #1844

Merge common code bases for TdsParserStateObject.cs (2) #1844

panoskj commented Nov 22, 2022 •

edited

Loading

bhugot Nov 22, 2022

panoskj Nov 22, 2022 •

edited

Loading

bhugot Nov 22, 2022

codecov bot commented Nov 22, 2022 •

edited

Loading

lcheunglci commented Nov 22, 2022

panoskj commented Nov 23, 2022 •

edited

Loading

bhugot left a comment

bhugot Nov 23, 2022

panoskj Nov 23, 2022

bhugot Nov 23, 2022

panoskj Nov 23, 2022

bhugot Nov 23, 2022

bhugot Nov 23, 2022

bhugot Nov 23, 2022

bhugot Nov 23, 2022

panoskj commented Nov 24, 2022 •

edited

Loading

bhugot commented Jan 18, 2023

lcheunglci commented Jan 19, 2023

Wraith2 commented Jun 20, 2023

JRahnama commented Jun 21, 2023

azure-pipelines bot commented Jun 21, 2023

Wraith2 commented Jun 21, 2023

JRahnama commented Jun 22, 2023

JRahnama commented Jun 22, 2023

David-Engel commented Jun 22, 2023

panoskj commented Jul 8, 2023 •

edited

Loading

		@@ -35,12 +50,6 @@ internal partial class TdsParserStateObject
		// Timeout variables
		internal bool _attentionReceived = false; // NOTE: Received is not volatile as it is only ever accessed\modified by TryRun its callees (i.e. single threaded access)

		@@ -51,45 +60,6 @@ internal partial class TdsParserStateObject

		internal bool _receivedColMetaData; // Used to keep track of when to fire StatementCompleted event.

Merge common code bases for TdsParserStateObject.cs (2) #1844

Merge common code bases for TdsParserStateObject.cs (2) #1844

Conversation

panoskj commented Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

panoskj Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2022 • edited Loading

Codecov Report

lcheunglci commented Nov 22, 2022

panoskj commented Nov 23, 2022 • edited Loading

bhugot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panoskj commented Nov 24, 2022 • edited Loading

bhugot commented Jan 18, 2023

lcheunglci commented Jan 19, 2023

Wraith2 commented Jun 20, 2023

JRahnama commented Jun 21, 2023

azure-pipelines bot commented Jun 21, 2023

Wraith2 commented Jun 21, 2023

JRahnama commented Jun 22, 2023

JRahnama commented Jun 22, 2023

David-Engel commented Jun 22, 2023

panoskj commented Jul 8, 2023 • edited Loading

panoskj commented Nov 22, 2022 •

edited

Loading

panoskj Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading

panoskj commented Nov 23, 2022 •

edited

Loading

panoskj commented Nov 24, 2022 •

edited

Loading

panoskj commented Jul 8, 2023 •

edited

Loading